Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add JetStream metadata to msg parsed from reply string #139

Merged
merged 5 commits into from
Sep 30, 2023

Conversation

simonhoss
Copy link
Contributor

@simonhoss simonhoss commented Sep 29, 2023

Hey

This PR adds the sequence and DateTime info to the msg.

Any feedback is appreciated.

@mtmk
Copy link
Collaborator

mtmk commented Sep 30, 2023

Thank you @simonhoss great to see this because it's implementing a missing feature in JetStream.

My initial thoughts are:

  • We don't want to penalize applications which are not using JetStream but using the reply subject for other patterns of messaging,
  • The feature really belongs to JetStream project,
  • NatsMsg is a struct so we need to be careful about enlarging it,
  • Have you considered calculating it on demand? (I'll also check with other client maintainers see how they're doing it)

@simonhoss
Copy link
Contributor Author

simonhoss commented Sep 30, 2023

Thanks for the feedback. When I started the implementation I didn't realize that there are two msg's in (NatsMsg and NatsJSMsg). With this new knowledge and your feedback, I was thinking about to implement it only in the NatsJSMsg with a lazy implementation. That means that the Seq and DateTime gets only calculated if someone wants to know it.

What do you think?

@mtmk
Copy link
Collaborator

mtmk commented Sep 30, 2023

Brilliant! Is there a way we can retrieve the new properties on NatsJSMsg without storing them on NatsMsg? Because NatsMsg is a struct it means every message needs to be copied so we want to keep it as small as possible 😉

BTW @simonhoss please sign your commits

@simonhoss
Copy link
Contributor Author

simonhoss commented Sep 30, 2023

I added a new approach, which doesn't touch the NatsMsg and only calculates the Sequence and DateTimewhen someone really consumes it.

Also my commits are now verified for this project 😉

Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 just needs a couple of changes to complete the implementation.

@simonhoss
Copy link
Contributor Author

simonhoss commented Sep 30, 2023

New version pushed. I think it should now parse both versions correct.

Also, Im not quite sure if struct makes sense for NatsJSMsgMetadata, but NatsJSMsg is also a struct, so I took the same.

Copy link
Collaborator

@mtmk mtmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thank you for the contribution @simonhoss 🥇

@mtmk
Copy link
Collaborator

mtmk commented Sep 30, 2023

Also @simonhoss please feel free to join https://slack.nats.io #dotnet channel if you haven't already done so. I'd love to see you there too 💯

@mtmk mtmk changed the title Add seq and datetime to msg parsed from reply string Add JetStream metadata to msg parsed from reply string Sep 30, 2023
@mtmk mtmk merged commit fabec64 into nats-io:main Sep 30, 2023
9 checks passed
@simonhoss
Copy link
Contributor Author

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants